Skip to content

Conversation

okhsunrog
Copy link

For correct serialization of decimal types into JSON we want them to be serialized as strings instead of numbers so they don't loose precision.

@github-actions github-actions bot added the arrow label Sep 26, 2025
@0x501D 0x501D requested review from askalt and novartole September 26, 2025 05:11
@askalt
Copy link

askalt commented Sep 26, 2025

Thank you for the patch!

We need to fix tests here:
https://github.com/tarantool/arrow-rs/blob/main/arrow-json/src/writer/mod.rs

Btw, does decimal deserializer properly works with input strings?

@okhsunrog
Copy link
Author

I use parse_decimal from arrow-cast/src/parse.rs and it works good

@askalt
Copy link

askalt commented Sep 26, 2025

I use parse_decimal from arrow-cast/src/parse.rs and it works good

Ok, it seems that deserializer also uses it https://github.com/tarantool/arrow-rs/blob/main/arrow-json/src/reader/decimal_array.rs#L57-L60, so it will be able to deserialize strings as decimals.

@novartole
Copy link

Thank you for the patch!

The last, but not least: let's squash commits keeping only meaningful ones (perhaps, reduce to one, but it's up to you) and add a bit of context about things are done.

The rest LGTM.

Add DecimalFormat enum with Number and String variants to control how
decimal values are serialized in JSON output. The default Number format
renders decimals as JSON numbers (e.g., 12.34), while String format
renders them as quoted strings (e.g., "12.34").

Changes:
- Add DecimalFormat enum to arrow-cast with Number (default) and String variants
- Add decimal_format field to EncoderOptions and FormatOptions
- Implement decimal formatting in DisplayIndexState for Decimal128/256
- Add WriterBuilder::with_decimal_format() configuration method
- Add tests for decimal arrays, lists, and dictionaries
- Fix lifetime elision warnings in BitChunks methods
@okhsunrog okhsunrog force-pushed the fix/decimal-json-string-serialization branch from 9ba3b11 to a0bb041 Compare October 13, 2025 15:35
@okhsunrog
Copy link
Author

@novartole thank you for the review! I squashed the commits and created a more meaningful commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants